Skip to content

MAVLink: refine reconnect STATUSTEXT (per-peer tracking, snapshot rate limit) - #11731

Open
xznhj8129 wants to merge 7 commits into
iNavFlight:maintenance-10.xfrom
xznhj8129:mav/reconnect-refine
Open

MAVLink: refine reconnect STATUSTEXT (per-peer tracking, snapshot rate limit)#11731
xznhj8129 wants to merge 7 commits into
iNavFlight:maintenance-10.xfrom
xznhj8129:mav/reconnect-refine

Conversation

@xznhj8129

@xznhj8129 xznhj8129 commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Follow-up to the reconnect STATUSTEXT handling merged in #11715, addressing review feedback on the reconnect-detection design.

The original commit tracked heartbeat presence with a single timestamp per physical port and sent an arming-reason snapshot on every judged reconnect. Review raised two soundness issues with that:

  • a steady peer on a port masks a newly joining peer behind the same port (their heartbeats keep the single timestamp fresh), and a peer failing over to another port is never noticed;
  • MAVLink leaves heartbeat rate channel-dependent, so a peer heartbeating slower than the 5 s gap threshold is classified as reconnecting on every beat, resending the arming notice each time.

Changes

  • Per-peer tracking. Heartbeat presence now lives in the route-table entry (sysid/compid), not per port. A steady peer no longer masks a new one; a peer moving to another port registers as a reconnect there. If the route table is full the new peer degrades gracefully to the pre-existing broadcast-only behaviour.
  • Snapshot rate limit. A 10 s per-port floor on arming snapshots, so a slow-heartbeat peer can't elicit a resend on every beat.
  • Dropped the enable-transition snapshot. It fired into a port nobody had connected to yet (boot) and duplicated the first-heartbeat snapshot on shared-port enable. Reconnect is now driven purely by the heartbeat signal; the enable transition still resets per-port one-shot state via the port reset path.

Testing

  • SITL builds clean with warnings-as-errors; mavlink_unittest 38/38 at this level (no new unit tests — behaviour is timing/multi-peer and covered by the live rig below).
  • Live SITL + pymavlink: existing reconnect regression 6/6, plus new cases — a 6.5 s-period peer is capped to 3 arming sends across 5 beats (was 5), and a second sysid joining a busy port gets its own snapshot while the steady peer produces none.

Built on the mav/04-streams-protocol content already in maintenance-10.x via #11715, so this shows a single commit.

@qodo-code-review

qodo-code-review Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@xznhj8129 xznhj8129 mentioned this pull request Jul 19, 2026
Merged
xznhj8129 added 7 commits July 25, 2026 20:54
@xznhj8129
Add MAVLink and MSP command-control parity
29eabf9 
- MAV_CMD_COMPONENT_ARM_DISARM and MSP2_INAV_ARM_DISARM through the
  normal arming path, succeeding only when the requested state is
  reached
- RTH via a temporary BOXNAVRTH source on the RC mode selector
  (activateRTHMode) instead of the failsafe/geozone forced-RTH latch;
  wired to MAV_CMD_NAV_RETURN_TO_LAUNCH, ArduPilot DO_SET_MODE RTL,
  MSP2_INAV_ACTIVATE_RTH, and Programming Framework operation 61.
  Cleared by a pilot flight-mode change or disarm
- QGC/ArduPilot pause: DO_SET_MODE Loiter/PosHold/Brake enters normal
  PosHold at the current position via a temporary BOXNAVPOSHOLD source
- Normal current-position LAND (transient waypoint, uploaded mission
  untouched) via MAV_CMD_NAV_LAND, MSP2_INAV_ACTIVATE_LANDING, PF op 62
- MAV_CMD_DO_SET_HOME through the native waypoint-0 backend
- MSP2_INAV_TIMESYNC returning the MAVLink TIMESYNC boot clock
- Temporary fixed-wing loiter-radius override: DO_REPOSITION.param3
  (meters) or int32 loiterRadius appended to MSP2_INAV_SET_GLOBAL_TARGET
  (cm); volatile, cleared on disarm/reboot, only active in PosHold
- SET_POSITION_TARGET_GLOBAL_INT / _LOCAL_NED guided handling
- MAV_CMD_CONDITION_YAW; explicit unsupported MAV_CMD_NAV_TAKEOFF stub
- GCSN OSD flight-mode element while GCS navigation is active

Unit slice: 81/81 passing.
@xznhj8129
Don't credit mission items for command-triggered landings
3efee9d 
A command-triggered landing (MAV_CMD_NAV_LAND / MSP direct land) borrows
NAV_STATE_WAYPOINT_RTH_LAND and the FW autoland FSM with a transient
waypoint, while activeWaypointIndex still points at whatever mission item
was last active. The unconditional reached-marking added for mission LAND
items would emit MISSION_ITEM_REACHED for that stale index and could mark
a loaded mission complete. Capture forcedLandingActivated before the
existing clears and skip the marking for commanded landings at both
finish sites.
@xznhj8129
Add MSP-over-MAVLink tunnel
dcf1603 
MSP transport over MAVLink TUNNEL (private payload type 0x8001,
MAVLink 2 only) so the Configurator can talk MSP over an existing
MAVLink telemetry link. Reuses the MSP parser/encoder through narrow
msp_serial seams; replies are fragmented and returned on the ingress
port only; MSPv1/v2 framing symmetry is preserved end-to-end. Reboot
post-processing is allowed; serial passthrough and ESC 4-way are
rejected before dispatch. Malformed payload lengths are dropped and
stale partial frames time out. Reply/frame buffers are file-scope, not
task stack.

Companion configurator branch adds the MAVLink Tunnel wireless option.
Full mavlink_unittest suite: 86/86 passing.

Final PR of the mavlink_multiport2 stack: tree now matches the feature
branch (remaining deltas are upstream maintenance-10.x drift only).
@xznhj8129
Add MAVLink user documentation and regenerate Settings.md
992f641 
docs/Mavlink.md documents the full MAVLink stack: multiport + routing,
datastream groups and CLI settings, identity/capabilities, mLRS
integration, supported outgoing/incoming messages and commands, mode
mappings, mission behavior and MSP parity gaps, MSP-over-MAVLink
tunnel, and high-latency mode. Updated from the development branch to
cover flight-mode-change STATUSTEXT notices and reworded for mainline.

docs/Settings.md regenerated from settings.yaml (picks up the per-port
mavlink_port{1-4}_* settings).
@xznhj8129
Add MSP-over-MAVLink tunnel smoke test
626f11f 
End-to-end live harness against a running SITL/FC MAVLink endpoint:
API version, FC variant/version, build info, EEPROM write, reboot over
the tunnel, and reconnect recovery after reboot.
@xznhj8129
docs: correct MAVLink documentation drift found in code audit
2a16c78 
- Document per-peer reconnect STATUSTEXT/arming re-announcement
- Fix STATUSTEXT severity: arming-disable and mode notices are NOTICE only
- Correct MSP-parity LAND/RTH rows: leg speed and RTH land flag are captured on upload, dropped on download
- Restrict stream-rate CLI settings to port 1 (ports 2-4 have none)
- Complete upload-translator list (RTH/SET_POI/SET_HEAD) and DO_REPOSITION param4 heading
- Note downloads always reply MISSION_ITEM_INT
- Fix dead MSP-Navigation-Messages.md link -> Navigation.md wp section
@xznhj8129
Refine reconnect detection: per-peer tracking, snapshot rate limit
9a4d419 
Review feedback on the reconnect STATUSTEXT commit:

- Track heartbeat presence per peer in the route table instead of one
  timestamp per port, so a steady peer cannot mask a newly joining one
  behind the same port, and a peer failing over to another port
  registers as a reconnect there. If the route table is full the new
  peer degrades to broadcast-only behavior.
- Rate-limit arming snapshots per port so a peer heartbeating slower
  than the gap threshold cannot elicit a resend on every beat.
- Drop the enable-transition snapshot: it fired into a port nobody had
  connected to yet (boot) and duplicated the first-heartbeat snapshot on
  shared-port enable. Reconnect is now driven purely by the heartbeat
  signal; the enable transition still resets per-port one-shot state via
  the port reset path.
@xznhj8129
xznhj8129 force-pushed the mav/reconnect-refine branch from 76e8e3e to 9a4d419 Compare July 26, 2026 01:05
@sensei-hacker

sensei-hacker commented Aug 3, 2026

Copy link
Copy Markdown
Member

Went through this one closely since it's specifically about reliability of the arming-state notification — a couple of things I noticed that both seem to reproduce the same class of issue this PR is fixing, wanted to check my understanding:

Route-table-full peers get no notification at all, not "broadcast-only" fallback
When mavlinkFindRoute() returns NULL (table full — MAVLINK_MAX_ROUTES with no eviction, only cleared by a full mavlinkRuntimeFreePorts() reinit), the whole reconnect-detection block is skipped for that peer, so mavlinkPortReconnected() never fires for it. The comment describes this as degrading "to the pre-existing broadcast-only behavior," but mavlinkSendArmingStatusText() (the broadcast path) only fires on disableFlags changing — if arming is already disabled and stays disabled, a peer joining after the table is full gets no notice at all, not a broadcast one. Is the intent for the route table to have eviction (e.g. LRU by last-heartbeat) so it can't permanently fill with stale peers, or is capacity expected to never realistically be hit in practice?

The new per-port snapshot floor can suppress a second peer's first-ever snapshot
mavlinkSendArmingStatusTextToPort()'s new 10s floor is keyed by mavPortStates[portIndex].lastArmingSnapshotMs — per port, not per peer. If peer A's heartbeat correctly triggers a snapshot on a shared port, and peer B (different sysid/compid) sends its own first-ever heartbeat on that same port within the next 10s, peer B's reconnect is correctly detected via its own route entry, but the resulting snapshot silently no-ops because of the floor peer A just reset — and there's no retry. That's the exact multi-peer-per-port scenario this PR's per-peer route-table change was written to fix, just reappearing at the delivery stage instead of the detection stage. Would scoping the floor to (port, sysid) — or storing it in mavlinkRouteEntry_t instead of mavlinkPortRuntime_t — avoid that, or is a shared per-port floor intentional here?

Removing the enable-transition mavlinkPortReconnected() call
resetMAVLinkPortRuntimeState() still resets the port's own one-shot/text state, but it doesn't touch route-table entries, and nothing else does either outside a full mavlinkRuntimeFreePorts() reinit. So if a shared port is disabled and quickly re-enabled, and the same peer's heartbeat resumes within the 5s gap window, its route entry's lastHeartbeatMs survives untouched — no firstHeartbeat/heartbeatGap/portChanged trips, so no fresh snapshot goes out despite the port having just been fully reset. Was dropping this call intentional on the assumption route-based detection would always catch it, or would it be worth invalidating the relevant route entry's timestamp on port disable to keep that guarantee?

Not trying to block on all of these — mostly want to understand whether the route-table-full and shared-floor cases are known/acceptable tradeoffs before flagging them as gaps. The per-peer keying itself (the core fix for the steady-peer-masking-a-new-one bug) looks correct and clean, and the millis()-wraparound-safe comparisons and the != 0 guard on the floor check are both handled right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@xznhj8129 @sensei-hacker